some proxies may add port number to X-Forwarded-For header. #533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent update enhances the HTTP handler's ability to accurately determine a client's IP address. It now interprets the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- xrayhttp/handler.go (1 hunks)
- xrayhttp/handler_test.go (1 hunks)
Additional comments: 2
xrayhttp/handler.go (1)
- 196-203: The
getHostfunction correctly splits the host and port from a given string and returns just the host part if the split is successful. This is a useful utility function for handling IP addresses that may include port numbers.xrayhttp/handler_test.go (1)
- 684-716: The tests added for the
TestClientIPfunction cover the new scenarios introduced by the changes inhandler.go. They include tests for handlingX-Forwarded-Forheaders with IPv6 addresses, multiple IP addresses, and IP addresses with ports. This ensures that theclientIPfunction is properly tested for the new parsing logic.
Comment on lines
+160
to
+194
| // clientIP returns the client IP address from the request. | ||
| // | ||
| // If the request passes through a proxy, clientIP parses | ||
| // the Forwarded header and the X-Forwarded-For header to | ||
| // find the client IP address. | ||
| // The clientIP always trusts the values of the Forwarded header | ||
| // and the X-Forwarded-For header. Please note | ||
| // that if these headers have been altered by an attacker, | ||
| // there is a possibility of returning incorrect results. | ||
| func clientIP(r *http.Request) (string, bool) { | ||
| // Parse Forwarded header. | ||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded | ||
| forwarded, err := forwardedheader.Parse(r.Header.Values("Forwarded")) | ||
| if err == nil && len(forwarded) > 0 { | ||
| ip := forwarded[0].For.IP | ||
| if ip.IsValid() { | ||
| return ip.String(), true | ||
| } | ||
| } | ||
|
|
||
| // Parse X-Forwarded-For header. | ||
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For | ||
| forwardedFor := r.Header.Get("X-Forwarded-For") | ||
| if forwardedFor != "" { | ||
| if idx := strings.IndexByte(forwardedFor, ','); idx > 0 { | ||
| forwardedFor = forwardedFor[:idx] | ||
| } | ||
| return strings.TrimSpace(forwardedFor), true | ||
| forwardedFor = strings.TrimSpace(forwardedFor) | ||
|
|
||
| // some proxies may add port number to X-Forwarded-For header. | ||
| // e.g. AWS ALB: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/x-forwarded-headers.html | ||
| return getHost(forwardedFor), true | ||
| } | ||
| ip, _, err := net.SplitHostPort(r.RemoteAddr) | ||
| return getHost(r.RemoteAddr), false | ||
| } |
There was a problem hiding this comment.
The clientIP function has been updated to handle cases where the X-Forwarded-For header includes port numbers. However, there is a potential issue with the getHost function call at line 191. It assumes that the X-Forwarded-For header will contain an IP address followed by a port, but this is not always the case. The getHost function should handle cases where there is no port number present.
Additionally, there is no validation to ensure that the IP address extracted is a valid format before returning it. This could lead to potential security issues if the header is manipulated.
func clientIP(r *http.Request) (string, bool) {
// Parse Forwarded header.
forwarded, err := forwardedheader.Parse(r.Header.Values("Forwarded"))
if err == nil && len(forwarded) > 0 {
ip := forwarded[0].For.IP
if ip.IsValid() {
return ip.String(), true
}
}
// Parse X-Forwarded-For header.
forwardedFor := r.Header.Get("X-Forwarded-For")
if forwardedFor != "" {
if idx := strings.IndexByte(forwardedFor, ','); idx > 0 {
forwardedFor = forwardedFor[:idx]
}
forwardedFor = strings.TrimSpace(forwardedFor)
// some proxies may add port number to X-Forwarded-For header.
// e.g. AWS ALB: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/x-forwarded-headers.html
- return getHost(forwardedFor), true
+ host, _ := getHost(forwardedFor)
+ if host == "" {
+ return "", false // or handle the error appropriately
+ }
+ return host, true
}
return getHost(r.RemoteAddr), false
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // clientIP returns the client IP address from the request. | |
| // | |
| // If the request passes through a proxy, clientIP parses | |
| // the Forwarded header and the X-Forwarded-For header to | |
| // find the client IP address. | |
| // The clientIP always trusts the values of the Forwarded header | |
| // and the X-Forwarded-For header. Please note | |
| // that if these headers have been altered by an attacker, | |
| // there is a possibility of returning incorrect results. | |
| func clientIP(r *http.Request) (string, bool) { | |
| // Parse Forwarded header. | |
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded | |
| forwarded, err := forwardedheader.Parse(r.Header.Values("Forwarded")) | |
| if err == nil && len(forwarded) > 0 { | |
| ip := forwarded[0].For.IP | |
| if ip.IsValid() { | |
| return ip.String(), true | |
| } | |
| } | |
| // Parse X-Forwarded-For header. | |
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For | |
| forwardedFor := r.Header.Get("X-Forwarded-For") | |
| if forwardedFor != "" { | |
| if idx := strings.IndexByte(forwardedFor, ','); idx > 0 { | |
| forwardedFor = forwardedFor[:idx] | |
| } | |
| return strings.TrimSpace(forwardedFor), true | |
| forwardedFor = strings.TrimSpace(forwardedFor) | |
| // some proxies may add port number to X-Forwarded-For header. | |
| // e.g. AWS ALB: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/x-forwarded-headers.html | |
| return getHost(forwardedFor), true | |
| } | |
| ip, _, err := net.SplitHostPort(r.RemoteAddr) | |
| return getHost(r.RemoteAddr), false | |
| } | |
| // clientIP returns the client IP address from the request. | |
| // | |
| // If the request passes through a proxy, clientIP parses | |
| // the Forwarded header and the X-Forwarded-For header to | |
| // find the client IP address. | |
| // The clientIP always trusts the values of the Forwarded header | |
| // and the X-Forwarded-For header. Please note | |
| // that if these headers have been altered by an attacker, | |
| // there is a possibility of returning incorrect results. | |
| func clientIP(r *http.Request) (string, bool) { | |
| // Parse Forwarded header. | |
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded | |
| forwarded, err := forwardedheader.Parse(r.Header.Values("Forwarded")) | |
| if err == nil && len(forwarded) > 0 { | |
| ip := forwarded[0].For.IP | |
| if ip.IsValid() { | |
| return ip.String(), true | |
| } | |
| } | |
| // Parse X-Forwarded-For header. | |
| // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For | |
| forwardedFor := r.Header.Get("X-Forwarded-For") | |
| if forwardedFor != "" { | |
| if idx := strings.IndexByte(forwardedFor, ','); idx > 0 { | |
| forwardedFor = forwardedFor[:idx] | |
| } | |
| forwardedFor = strings.TrimSpace(forwardedFor) | |
| // some proxies may add port number to X-Forwarded-For header. | |
| // e.g. AWS ALB: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/x-forwarded-headers.html | |
| host, _ := getHost(forwardedFor) | |
| if host == "" { | |
| return "", false // or handle the error appropriately | |
| } | |
| return host, true | |
| } | |
| return getHost(r.RemoteAddr), false | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests